Skip to content

Comments

initial draft#1

Merged
AdirAmsalem merged 5 commits intomainfrom
first-draft
Aug 17, 2025
Merged

initial draft#1
AdirAmsalem merged 5 commits intomainfrom
first-draft

Conversation

@AdirAmsalem
Copy link
Contributor

No description provided.

Comment on lines +21 to +24
const stream = await navigator.mediaDevices.getUserMedia({
audio: true,
video: { frameRate: 14 }
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we maybe have it a part of SDK?
Because we probably want to control the fps (e.g. in Safari 14 FPS => 15 FPS and its high) and there were a lot of pitfalls in it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should abstract the entire navigator.mediaDevices.getUserMedia() call or the full video configuration - clients should be able to control and adjust these to their specific needs.

That said, since we do care about the frameRate (especially its max value) and possibly the max width/height and/or aspectRatio of the stream, we could expose these as helpers, verify the inputs, or cap them at our allowed limits - and add an error or warning if needed.

wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In terms of a backend I am torn between forcing max fps (i.e. dropping frames which arrive too early) or allowing any fps.
The former solution might be too strict (there is no problem if sometimes frames arrive a bit earlier or a bit late, but if we drop frames forcefully), hence we stick to the latter.

@AdirAmsalem
Copy link
Contributor Author

Since we're not publishing it right now, I'm going to merge it and handle all comments/feedback in follow-up PRs.

@AdirAmsalem AdirAmsalem marked this pull request as ready for review August 17, 2025 15:52
@AdirAmsalem AdirAmsalem merged commit c983dc5 into main Aug 17, 2025
1 check passed
@AdirAmsalem AdirAmsalem deleted the first-draft branch August 17, 2025 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants